Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport: v21.0.x rc.2 #6115

Merged

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Backports a set of 6 PRs needed in rc.2

What was done?

Backported PRs with labels

How Has This Been Tested?

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

… user, http-code, time of running

1a691bd feat: add logging for RPC HTTP requests: command, user, http-code, time of running (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently there is no way to gather stats for http rpc in dash core. This PR aim to change it.

  ## What was done?
  Implemented some basic stats for each RPC request:
   - rpc command
   - flag "is external"
   - http status
   - time to serve query (rpc time, not http time)

  ## How Has This Been Tested?
  See new logs:
  ```log
  [httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=false status=200 elapsed_time_ms=0
  [httpworker.2] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=quorum is_external=false status=500 elapsed_time_ms=0
  [httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command= is_external=false status=401 elapsed_time_ms=0
  [httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=true status=200 elapsed_time_ms=28
  [httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=operator command=getbestblockhash is_external=false status=200 elapsed_time_ms=0
  ```

  ## Breaking Changes
  N/A
  It doesn't change behavior of rpc server and http server.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 1a691bd

Tree-SHA512: b62fceedb9a901e87c23c4aa6e6dfa7226d44da84a081ea245b40e7ff887103302147cebe0f7ff90bf9c8d4fa9ecafbaa6f25f39d2008f62c4f2beeef2877b57
…lets

c944908 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov)
685cf34 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  As [noticed by kwvg](dashpay#6090 (review)), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue.
  dashpay/dash-issues#59

  ## What was done?
  Removed default argument "bool mixing = false" from WalletStorage interface,
  Refactored a logic of CWallet::IsLocked to make it shorter, clearer and unified with bitcoin.

  ## How Has This Been Tested?
  A. Run Dash-Qt with descriptor wallet, run mixing, enter passphrase.
  The wallet is partially unlocked (for mixing only) - possible to see yellow lock in status. Mixing happens.
  B. Open "send transaction dialog", try to send a transaction: the app asks password to unlock wallet as expected.

  Though, I am not sure how exactly to test that **all** rpc are indeed locked when descriptor wallet is unlocked for mixing only.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    LGTM, ~utACK~ light ACK c944908
  kwvg:
    ACK c944908
  PastaPastaPasta:
    utACK c944908

Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c
…r descriptor wallets

a42e9df fix: createwallet to require 'load_on_startup' for descriptor wallets (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPC `createwallet` has changed list of arguments: `createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )` since dashpay#5965
  `load_on_startup` used to be an argument 5 but now it has a number 6. Both arguments 5 and 6 are boolean and it can confuse an user: they may even not notice that something wrong when it meant to be "load on startup" but got "descriptor wallet" which is not expected.

  See also previous attempt to resolve issue: dashpay#6029

  ## What was done?
  To prevent confusion if user is not aware about this breaking changes, the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup. This requirement can be removed when major amount of users updated to v21.

  ## How Has This Been Tested?
  Run unit/functional tests

  Tested with CLI:
  ```
  $ createwallet "tmp-create" true true "" false true
  RPC createwallet would not accept creating descriptor wallets without specifying 'load_on_startup' flag. It is required explicitly in v21 due to breaking changes in createwallet RPC (code -8)
  $ createwallet "tmp-create" true true "" false true true
  {
    "name": "tmp-create",
    "warning": "Empty string given as passphrase, wallet will not be encrypted.\nWallet is an experimental descriptor wallet"
  }
  ```

  ## Breaking Changes
  You can't more pass 'descriptor=NN' without  dashpay#5965 which has not been released yet.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK a42e9df
  PastaPastaPasta:
    utACK a42e9df
  thephez:
    utACK a42e9df

Tree-SHA512: bf57fed40d04a32e756e4f8bfabbe39c0cbf87275546c92f4efc19523bc3c5dd3ddc5a884d67285971dc301a6c5808bef979db52c35645ca2db0810046fe1bdc
…turn enum in CL verifying code

0133c98 feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1 refactor: replace static private member method to static method (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.

  This PR aims to fix this issue, as requested by shumkov for needs of platform:

  > mailformed signature and can’t verify signature due to unknown quorum is the same error?
  > possible to distingush ?

  ## What was done?
  Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
  The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`

  ## How Has This Been Tested?
  Functional test `feature_llmq_chainlocks.py` is updated

  ## Breaking Changes
  `submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 0133c98
  PastaPastaPasta:
    utACK 0133c98

Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
…s for platform needs

85abbb9 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  dashpay/dash-issues#66
  dashpay/dash-issues#65

  ## What was done?
  Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see dashpay#6052 dashpay#6051 dashpay#6055 and other related PRs

  This PR makes whitelist feature to be compatible with composite commands.

  Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.

  Platform at their side can use config such as this one (created based on shumkov's example):
  ```
  rpc: {
            host: '127.0.0.1',
            port: 9998,
            users: [
              {
                user: 'dashmate',
                password: 'rpcpassword',
                whitelist: null,
                lowPriority: false,
              },
              {
                username: 'platform-dapi',
                password: 'rpcpassword',
                whitelist: [],
                lowPriority: true,
              },
              {
                username: 'platform-drive-consensus',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
                lowPriority: false,
              },
              {
                username: 'platform-drive-other',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
  ],
                lowPriority: true,
              },
            ],
            allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
          },
  ```

  ## How Has This Been Tested?
  Updated functional tests, see commits

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    LGTM, utACK 85abbb9
  PastaPastaPasta:
    utACK 85abbb9

Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
…says there is a default that doesn't exist

60e36b7 fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  This default doesn't actually exist in code.

  ## What was done?
  Remove default from text

  ## How Has This Been Tested?

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 60e36b7

Tree-SHA512: 658eb2cf101d0450619461b7ffe6069de5c04c1fdcca216713f9374cc1e60413ec9b96c3a2931fb69a4c2f8277dd6677100270960a58197da3b92dffb1e9e327
…msign

2db69d7 chore: add release notes for "quorum platformsign" (Konstantin Akimov)
283c5f8 feat: create new composite command "quorum platformsign" (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  It splits from dashpay#6100
  With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only.

  ## What was done?
  Implemented a new composite command "quorum platformsign"

  This composite command let to limit quorum type for signing for case of whitelist.
  After that old way to limit platform commands can be deprecated - dashpay#6105

  ## How Has This Been Tested?
  Updated a functional tests to use platform signing for Asset Unlocks feature.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 2db69d7
  PastaPastaPasta:
    utACK 2db69d7

Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
@PastaPastaPasta PastaPastaPasta added this to the 21 milestone Jul 15, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM db82817

@DashCoreAutoGuix
Copy link

Guix Automation has began to build this PR tagged as v21.0.0-devpr6115.db828177. A new comment will be made when the image is pushed.

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, utACK db82817

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK db82817

@DashCoreAutoGuix
Copy link

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.0.0-devpr6115.db828177. The image should be on dockerhub soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants